Skip to content

fix(projects): allow owners to delete agent projects - #6533

Open
wesbillman wants to merge 3 commits into
mainfrom
carl/fix-agent-owned-project-deletion
Open

fix(projects): allow owners to delete agent projects#6533
wesbillman wants to merge 3 commits into
mainfrom
carl/fix-agent-owned-project-deletion

Conversation

@wesbillman

Copy link
Copy Markdown
Collaborator

Summary

  • expose project deletion when the current user directly owns the project or owns/manages its author agent
  • publish a viewer-signed, project-only tombstone after fetching the exact live head, then detect concurrent replacement
  • honor relay-authorized owner-signed tombstones in desktop project read models

Testing

  • pre-push hook: desktop Biome, TypeScript, 5,358 desktop tests, Rust tests, Tauri checks, and file-size ratchet passed at a36f7a4cf
  • cargo check -p buzz-test-client --tests
  • relay E2E coverage added as ignored; not run against a live relay

Co-authored-by: Wes <wesbillman@users.noreply.github.com>
Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman
wesbillman requested a review from a team as a code owner August 22, 2026 00:45

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Requesting changes for one blocking user-visible authorization defect:

P1: Do not expose managed-agent deletion unless the production signer can exercise that authority. canDeleteProject treats every locally managed agent record as sufficient capability via managedAgentPubkeys.has(owner), including legacy/imported records that legitimately have no NIP-OA attestation. But deleteProject always uses signRelayEvent, which signs with the current human identity. The relay accepts that human-signed tombstone only when its database already contains the NIP-OA owner mapping. For a locally managed legacy/imported agent without that mapping, the UI enables Delete and confirmation deterministically fails with must be event author.

Either sign this narrowly constrained tombstone with the managed agent key (the existing project_owner_identity path demonstrates that capability), or stop treating local management alone as deletion authority and expose the action only for relay-verifiable ownership. Please also add coverage through the production signer/relay authorization boundary; the current injected signer/publisher test stubs away the rejection.

The relay authorization, tombstone race handling, and project read-model suppression otherwise look sound at a36f7a4cfca6c66c54a80e4586c130f490031738.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES
Reviewed: f7942167372501576c9f0f589cf2c166882668bb..a36f7a4cfca6c66c54a80e4586c130f490031738
Risk: critical — destructive, identity-bound relay authorization and failure recovery.

Two material issues remain:

  1. P1 — bind the Delete affordance to authority the production signer can exercise. desktop/src/features/projects/projectDeletion.ts:34-39 treats managedAgentPubkeys.has(owner) as deletion authority, although useProjectDeletionAccess.ts:17-25 loads that machine-local list without binding it to the active identity. The operation then signs with the current human identity (projectDeletion.ts:79-84,100-107), while the relay accepts that signer only when its community database says the human owns the agent (crates/buzz-relay/src/handlers/side_effects.rs:249-258). The existing ownership helper explicitly documents that local managed-agent records can diverge from relay ownership (desktop/src/features/profile/lib/identity.ts:134-148). Thus an imported/legacy agent without NIP-OA ownership—or another human identity on the same installation—gets an enabled Delete action in Projects and the sidebar, then deterministically receives must be event author. Remove local-list presence as capability evidence, or perform this constrained deletion with authority that actually owns the author agent. Add a regression spanning the production signer/relay authorization seam; the injected signer/publisher test cannot catch this mismatch.

  2. P2 — reconcile the project cache when publish acknowledgement is uncertain. RelayClient.publishEvent may time out after the relay has durably accepted the tombstone (desktop/src/shared/api/relayClientSession.ts:710-755). deleteProject then exits before its verification fetch (desktop/src/features/projects/projectDeletion.ts:103-113), and useDeleteProjectMutation removes/refetches only in onSuccess (desktop/src/features/projects/hooks.ts:964-975). Both confirmation flows close on failure (desktop/src/features/projects/ui/ProjectCards.tsx:424-428, desktop/src/features/sidebar/ui/SidebarProjectsSection.tsx:238-260). The result is a failure toast plus a still-actionable project from the five-minute cache; retry reports that no live head exists, still without reconciling the durable outcome. Invalidate/refetch the projects query on uncertain failure/settlement and cover accepted publish + lost ACK → deleted project absent. The timeout copy should not assert failure when the outcome is unknown.

The relay's community-scoped coordinate authorization, timestamp-dominating tombstone, project-only target, concurrent replacement detection, and the AlertDialog destructive confirmation looked sound in the reviewed paths. No unrelated schema, identity-storage, or release scope was introduced.

Validation at exact clean head: just desktop-typecheck passed; full just desktop-test passed (5,358/5,358); cargo check -p buzz-test-client --tests passed; git diff --check passed. CI relay/integration, smoke E2E, macOS build, Rust lint, security, cross-compile, and Docker jobs passed when checked. The Unit Tests job failed in sherpa-onnx-sys native static-library discovery after buzz-core passed 307/307; Desktop Core and Windows Rust were still running.

Residual risk: the ignored live-relay E2E and a native destructive UI journey were not run. Existing E2E does not cover the production Desktop signer or lost-ACK reconciliation.

wesbillman and others added 2 commits August 22, 2026 08:30
Co-authored-by: Wes <wesbillman@users.noreply.github.com>
Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
…project-deletion

Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Co-authored-by: Wes <wesbillman@users.noreply.github.com>
Co-authored-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman

Copy link
Copy Markdown
Collaborator Author

Addressed the requested changes in fe102ad6f and merged current main at 6c40c2f0b:

  • deletion affordance now requires direct ownership or relay-verifiable NIP-OA profile ownership; local managed-agent presence is no longer treated as authority
  • deletion timeout now reports an uncertain outcome and the mutation awaits project-query invalidation on both success and failure, with lost-ACK cache regression coverage

The failed Unit Tests job was unrelated infrastructure (sherpa-onnx-c-api missing during buzz-voice compilation); I reran the failed job after pushing. Pre-push passed at 6c40c2f0b (5,361 desktop tests, Rust tests, Tauri checks, TypeScript, Biome, and file-size gate).

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: COMMENT — the two previously blocking code defects are resolved at this SHA, but merge clearance is withheld pending a mergeable branch and clean required CI.
Reviewed: f99532585a0715bac73b4a6361a9b4966bdb5095..6c40c2f0bc48d5153ffc03eaa25adbf5dbf3c353 (exact head 6c40c2f0bc48d5153ffc03eaa25adbf5dbf3c353)
Risk: critical — destructive, identity-bound relay authorization with uncertain network outcomes.

The integrated re-review clears both prior findings:

  1. Signer authority is now truthful. desktop/src/features/projects/projectDeletion.ts:26-37 grants Delete only to the coordinate author or the human identified by the author’s cryptographically verified NIP-OA profile. Machine-local managed-agent presence is no longer capability evidence. Both Projects and sidebar use this decision (ProjectsOverviewItems.tsx:77-88,104-119; SidebarProjectsSection.tsx:124-160,310-335). The profile owner derives from verified NIP-OA evidence (desktop/src-tauri/src/nostr_convert.rs:64-79) and matches the relay’s community-scoped is_agent_owner authorization (crates/buzz-relay/src/handlers/side_effects.rs:237-259). The negative regression at projectDeletion.test.mjs:28-34 rejects an unverified viewer.

  2. Accepted deletion with a lost ACK now reconciles. Timeout copy reports an uncertain outcome (projectDeletion.ts:101-105), while projectDeletionMutation.ts:9-23 returns an onSettled invalidation for success and failure. The active-observer regression at projectDeletionMutation.test.mjs:19-47 proves the rejected mutation triggers a second fetch and replaces stale [project] state with []; independent mutation checks showed the regression fails when onSettled is removed.

The remaining destructive path is bounded correctly: the tombstone targets only project.projectAddress, dominates the fetched live head, and detects a surviving concurrent replacement (projectDeletion.ts:40-51,83-111). Relay authorization remains tenant-scoped, and the read model applies relay-accepted tombstone timestamps (projectModels.ts:389-427). The confirmation UI remains an AlertDialog with explicit irreversible scope, Cancel, destructive action, and pending disablement (ProjectCards.tsx:373-438; SidebarProjectsSection.tsx:395-447). The 13-file diff introduces no schema, migration, identity-storage, or release changes and remains aligned with VISION.md and VISION_PROJECTS.md ownership/tenant boundaries.

Exact-head validation:

  • just desktop-test — PASS, 5,400/5,400.
  • just desktop-typecheck — PASS.
  • Targeted authority and lost-ACK regressions — PASS, with both material fixes mutation-proven.
  • cargo check -p buzz-test-client --tests — PASS.
  • git diff --check f99532585a0715bac73b4a6361a9b4966bdb5095..HEAD — PASS.
  • GitHub’s relay E2E, backend integration, Desktop integration, Desktop Core, macOS build, Rust lint, security, cross-compile, and release-candidate checks passed at this head.

Integration blockers: GitHub still has red Unit Tests, Desktop Smoke E2E (4), and aggregate Desktop checks. The unit failure is unchanged buzz-voice native discovery (sherpa-onnx-c-api missing); smoke shard 4 has one persistent workflow-controls failure plus seven workflow/virtualization flakes, outside this PR’s changed paths. GitHub earlier reported the branch dirty; final freshness queries returned mergeability unknown, not proof of a clean merge. Rebase/resolve as needed, rerun required checks, and refresh review on the resulting head. Naturally, GitHub chose necromancy instead of a stable answer.

Residual risk: the ignored live-relay owner-deletion E2E and a native destructive GUI journey were not run. Confirmation focus/rendering and a real transport-level lost-ACK are therefore not newly runtime-proven; deterministic state-machine coverage and passing relay/integration lanes are the available evidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants